Skip to content

fix(library): stop deleted songs coming back - #527

Merged
thcp merged 1 commit into
fix/520-registry-phantom-jobfrom
fix/521-deleted-songs-return
Aug 31, 2026
Merged

fix(library): stop deleted songs coming back#527
thcp merged 1 commit into
fix/520-registry-phantom-jobfrom
fix/521-deleted-songs-return

Conversation

@thcp

@thcp thcp commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes #521. Targets fix/520-registry-phantom-job, not 0.16.1 directly -- it builds on that branch's hardening of restore(). Merge #526 first.

Reported on macOS: a song deleted by clearing the trash, or via Settings -> "Reset app data", comes back later.

Root cause

restore() adopts any job-shaped directory it finds. That is correct for a library whose registry was lost, and wrong for a job the user deleted whose files outlived the delete. Nothing on the server knew the difference, so the only thing standing between a failed delete and a resurrected song was a client-side tombstone -- which "Reset app data" wipes on its way out.

That is why the reset path was the worst one: it destroys the only defence, then trusts an unconditional {"ok": true}.

Changes

Server keeps its own deletion record. mark_deleted() records an id; orphan recovery skips it; persist() writes it alongside the jobs. _prune_deleted() forgets a record once its directory is finally gone, so the set stays bounded instead of growing for the life of the install. With this, the client tombstone becomes belt-and-braces rather than load-bearing.

_rmtree_job reports its outcome instead of logging and returning None, and retries once. On macOS the usual failure is Finder or Spotlight creating a .DS_Store between rmtree's scan and its final rmdir, which leaves "Directory not empty" on a directory that is about to be empty again -- a retry clears exactly that.

delete_job records the deletion either way, and returns an error when files remain rather than reporting success.

reset_all returns what it could not remove and records the survivors so they cannot be re-adopted. /api/reset now returns {"ok": true, "undeleted": N}.

Client awaits both halves. markJobsDeleted is awaited before the purge, and the DELETE calls are awaited with failures surfaced through notifyFailure. Both were fire-and-forget with .catch(() => {}). The button is disabled while it runs.

Verification

New tests/test_deleted_jobs_stay_deleted.py, 7 tests. Confirmed not vacuous -- removing the orphan-recovery skip makes 2 fail:

FAILED test_a_deleted_job_is_not_re_adopted
FAILED test_reset_records_survivors_so_they_cannot_come_back

The first test in the file deliberately pins the behaviour we must not break: an orphan directory that was never deleted is still adopted.

ruff check     All checks passed
ruff format    95 files already formatted
pytest         911 passed, 2 failed (the pre-existing ogg pair, unchanged)
npm test:js    48/48
node --check   catalog.js, i18n.js OK

Two things reviewers should weigh

delete_job now returns 500 when files remain, after having already removed the registry entry and recorded the deletion. The row goes and stays gone -- which is what the user asked for -- but the response is an error so the failure is visible. The alternative was keeping the row and forcing a retry; that felt worse given the .DS_Store race usually clears on retry anyway.

/api/reset's response shape changed. tests/test_reset.py was updated for the new undeleted field. Any other consumer of that endpoint would need the same.

Still unconfirmed

The macOS .DS_Store race is a hypothesis, not a verified root cause. Checking jobs/ for surviving directories and backend.log for reset: could not remove after a reset that appeared to work would settle it. The fix does not depend on that being the trigger -- the deletion record closes the resurrection path whatever caused the delete to fail.

Reported on macOS: a song deleted by clearing the trash, or by Settings ->
"Reset app data", returns later. Deletion had two halves and both swallowed
their failures, so several independent paths produced the same symptom.

The root cause is that restore() adopts any job-shaped directory it finds.
That is right for a library whose registry was lost and wrong for a job the
user deleted whose files outlived the delete. Nothing on the server knew the
difference, so the only thing standing between a failed delete and a
resurrected song was a client-side tombstone -- which "Reset app data" wipes
on its way out.

The registry now keeps its own deletion record. Orphan recovery skips those
ids, so a directory that survives a delete stays gone regardless of what the
client does. Records are pruned once their directory is finally absent, so the
set stays bounded rather than growing for the life of the install.

_rmtree_job reports whether the files actually went away instead of logging
and returning None, and retries once: on macOS the usual failure is Finder or
Spotlight creating a .DS_Store between rmtree's scan and its final rmdir,
which leaves "Directory not empty" on a directory that is about to be empty
again. delete_job records the deletion either way and tells the caller when
files remain.

reset_all returns what it could not remove and records the survivors, and
/api/reset reports the count instead of an unconditional {"ok": true} that the
frontend took as licence to wipe its own tombstone.

On the client, the tombstone write and the DELETE calls are both awaited. They
were fire-and-forget with .catch(() => {}), so quitting soon after clearing the
bin lost the tombstone, and a delete that failed -- a 409 on a job stuck in
"queued", a 500 when files could not be removed -- was invisible. Failures now
surface through notifyFailure, and the button is disabled while it runs.

Verified: removing the deletion record makes the two resurrection tests fail.

Refs #521
@thcp
thcp marked this pull request as ready for review August 31, 2026 21:02
@thcp
thcp merged commit b8676e0 into fix/520-registry-phantom-job Aug 31, 2026
8 checks passed
@thcp
thcp deleted the fix/521-deleted-songs-return branch August 31, 2026 21:03
thcp added a commit that referenced this pull request Sep 1, 2026
…om the bin, and 14 more fixes (#541)

Ships the 0.16.1 fix set to `main`. Eighteen merged changes: sixteen
fixes from a pre-release bug scan, one refactor, and one feature.

## Why this release exists

#506 fixed the macOS AppleDouble bug but is unreleased, so every macOS
0.16.0 user still has a broken runtime: `import matplotlib.pyplot`
fails, which kills `allin1_infer`, which kills automatic song sections.
Overwriting the 0.16.0 assets would not reach them, the runtime
reinstall is gated on a version-string comparison
(`desktop/ui/setup.js`), so a new version is the only route.

## What is in it

**Data loss and user-visible bugs**

- **#509** `settings.json` was written with `write_text` (truncate, then
write), and `_load` could not tell a torn file from a first run. A real
user lost `port` and `allow_network` from both the file and its mirror.
Now atomic, and a corrupt file is preserved as
`settings.json.corrupt-<ts>` and recovered from the mirror.
- **#521** Deleted songs came back. `reset_all` swallowed per-directory
failures, `/api/reset` reported unconditional success, the frontend then
wiped its own tombstone, and `restore()` re-adopted every surviving
directory on the next start. A server-side deletion record closes it.
- **#542** Trashed songs came back too, by a different route.
`addTrackToLibrary` deduplicates by source URL, and when the match was
in the Trash it deleted the catalog entry without deleting the job. The
directory and its registry record outlived their only reference, and
`syncWithServer` re-adopted the orphan on the next launch. Any second
job sharing the URL was enough to trigger it. Found while testing this
release on Windows.
- **#515** The footer scrub bar did nothing, and "set loop in at
playhead" always wrote 0, `main.js` drove the silent multitrack while
`audioEngine` owned the clock.
- **#520** A cancel landing between the queue worker's pop and claim
stranded a job at `queued` forever: invisible, still counted against
capacity, source file never freed, re-queued on every restart. Also, a
malformed `registry.json` raised an uncaught `AttributeError` at import
and the backend never started.

**Security**

- **#510** The in-app updater installed an executable from a
WebView-supplied URL with no host allowlist, checked against a SHA from
the same caller.
- **#511** Fork PRs executed arbitrary code on the self-hosted runners,
the same machine that builds and signs releases.
- **#518** Linux FFmpeg was downloaded, chmod +x and executed with no
integrity check at all.
- **#517** Deno pulled from `releases/latest` unpinned and unverified
into every image; releases could publish without updater assets and stay
green; a failed CPU-torch install was silently ignored, shipping a
non-CPU torch in the CPU zip.

**Robustness**

- **#508** A regression in #506 itself: the per-entry unpack loop lost
`Archive::unpack`'s directory deferral, so a read-only directory member
would fail extraction outright.
- **#512** `end` had no upper bound, reaching a multi-GB `np.zeros` on
the event loop; the body-size guard covered two paths and was bypassed
by chunked encoding.
- **#513** SSE slots leaked permanently when a client disconnected
before the body started, 200 of those and every progress stream 503s
with nothing connected.
- **#514** Worker teardown sat outside the `finally`, so an exception
left a poisoned CUDA worker warm; cancel was dropped before the CPU
fallback, costing 10+ minutes.
- **#516** `child_output_with_timeout` never drained child pipes until
exit, deadlocking any chatty child.
- **#519** Cancellation never reached several pipeline subprocesses, and
two of three workers never armed the parent-PID watchdog, so a
Force-Quit orphaned a GPU-holding process.

**Presentation**

- **#543** Three of the eight logos listed in the We Recommend dialog
had no file behind them. Analog4Lyfe, Empress Effects and Thomann showed
a broken card on every install, with a 404 in the backend log each time
the dialog opened.
- **#544** That dialog was one flat list of twelve entries with no order
a reader could perceive. It is grouped into five categories now,
`r/bass` is added so the app matches the README, and the twelve
descriptions move out of hardcoded English into the i18n layer across
all ten language tables.

**Feature**

- **#538** Loop regions can be adjusted rather than redrawn, drag either
edge independently, or drag the region to slide it. From discussion
#507.

## Verification

```
ruff check       All checks passed
ruff format      101 files already formatted
pytest tests/    947 passed, 2 failed
npm run test:js  11/11
playwright       85 passed
i18n audit       clean
cargo fmt        clean
cargo clippy     0 errors
cargo test       60 passed, 1 failed
```

Every fix was individually confirmed present on this branch by grepping
for its introduced symbol, rather than trusting merged state, which is
how #527 was caught having merged into an orphaned branch instead of the
release branch (recovered as #540).

**The 3 failures are all pre-existing and reproduce on `main`:**

- `test_all_stems_zip_ogg` and `test_ogg_is_still_streamed`, most likely
a local ffmpeg built without libvorbis. Unconfirmed, not filed.
- `a_free_port_is_granted_as_asked`, a known parallel-execution flake.
It probes port 21000 with `std::net::TcpListener` (which sets
`SO_REUSEADDR`) then asserts `claim_port` (socket2, without it) binds
the same port. Not equivalent, and there is a TOCTOU gap.

## Tested on a real Windows build

The branch was packaged with `make-portable.ps1 -CpuOnly` and driven by
hand. What that covered:

- **#509** port and `allow_network` survived a quit and relaunch, in
both the portable file and the AppData mirror.
- **#521** a hard-deleted job stayed deleted across a restart. The
deletion record self-pruned once the directory was gone, which is the
designed behaviour.
- **#542** a trashed job stayed in the Trash across a restart, and again
across a fresh import of the same URL. Both cases fail on 0.16.0.
- **#520** a cancel while queued removed the job and freed its capacity
slot, with nothing stranded.
- **#519 / #514** a cancel mid-separation wiped the partial output and
the queued job started immediately, with no leftover worker process. A
quit mid-separation left zero orphaned `python.exe`, and the interrupted
job resumed once with `resume_attempts: 1`.

## Two things reviewers should know before tagging

**`make-portable.ps1` now has a parser pass and a real run.** It parses
clean under Windows PowerShell 5.1, and the full CPU-only Windows
package built end to end from this branch, exit code 0. The earlier
caveat here is resolved.

**Nothing compiles the Linux Rust shell until release time** (#531).
`ci.yml` never invokes cargo; Linux Rust is built only by
`linux-release.yml`. #518 ships a change living entirely inside
`#[cfg(all(unix, not(target_os = "macos")))]`, which could only be
type-checked by temporarily widening the cfg gate. If the release build
fails, look there first.

**#510 is not manually testable.** The update check runs automatically
at startup and only surfaces when GitHub has a newer non-prerelease
release, so the host allowlist in the installer is not reachable by
hand. It exercises itself at release time.

## Not included

The Unraid template still pins `0.16.0`, deliberately left for a
separate decision.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant